cuslide2: add pyramidal OME-TIFF semantics and nvImageCodec 0.9 readiness - #1122
cuslide2: add pyramidal OME-TIFF semantics and nvImageCodec 0.9 readiness#1122cdinea wants to merge 17 commits into
Conversation
854582c to
70e0e73
Compare
Update nvImageCodec dependency constraints to 0.9.x, add parser/decoder compatibility fallbacks for 0.9 runtime behavior, and relax legacy SubfileType assumptions so stacked OME-style TIFF pages load successfully in cuslide2 validation.
|
/ok to test 57aa615 |
| exec_params.device_allocator = nullptr; | ||
| exec_params.pinned_allocator = nullptr; | ||
| exec_params.max_num_cpu_threads = compute_max_decoder_threads(); | ||
| exec_params.max_num_cpu_threads = 0; |
There was a problem hiding this comment.
I'm wondering why we're setting this to 0 now.
There was a problem hiding this comment.
Yeah this would recover the overcontention issues which were fixed in the last release.
There was a problem hiding this comment.
Good catch — unintentional, and Lev is right that it reintroduces the over-contention regression.
nvimgcodec.h documents max_num_cpu_threads = 0 as "default value equal to number of cpu cores", so this decoder was silently taking a full-core CPU threadpool. That was harmless when it was CPU-only and metadata-only, but it now backs the primary decode path (NVIMGCODEC_DEVICE_CURRENT, all backends), so with N dataloader workers you get N x cores threads.
The 0 is a leftover from the pre-promotion shape of this code; the rebase dropped compute_max_decoder_threads() along with it, which is the ~76-line shift in this hunk.
Fixed in 9bd44b8 — heuristic is back to min(max(1, hardware_concurrency / 4), 8), still overridable via CUCIM_MAX_DECODER_THREADS (>0 = exact count, 0 = explicit opt-in to the nvImageCodec default).
| // Current limitation (nvImageCodec v0.6.0): | ||
| // - codec_name returns "tiff" (container format) not compression type | ||
| // - Individual TIFF tags not exposed through metadata API | ||
| // - Only vendor-specific metadata blobs available (MED_APERIO, MED_PHILIPS, etc.) | ||
| // |
There was a problem hiding this comment.
I am wondering if nvImageCodec v0.9 still has these limitation.
There was a problem hiding this comment.
We do support generic tags in the API, here is python example: https://docs.nvidia.com/cuda/nvimagecodec/samples/metadata.html#Generic-tiff-tag-reading
iirc this was already supported in 0.8? Lev can know more details on this and how to querry the tags
There was a problem hiding this comment.
This has been supported since 0.8, but only with a GPU decoder.
There was a problem hiding this comment.
You're right, and thanks @mkepa-nv for the pointer — the comment was stale. Individual TIFF tags are queried unconditionally via NVIMGCODEC_METADATA_KIND_TIFF_TAG in extract_tiff_tags(), which runs on the line directly above, so the claim contradicted the adjacent code.
Removed in 9bd44b8. I kept only the part that's still true: codec_name reports the container format rather than the compression type, so compression is inferred from the tags below.
| // WORKAROUND for nvImageCodec 0.6.0: Philips TIFF metadata limitation | ||
| // ======================================================================== | ||
| // nvImageCodec 0.6.0 does NOT expose: | ||
| // 1. Individual TIFF tags (SOFTWARE, ImageDescription, etc.) | ||
| // 2. Philips format detection for some files | ||
| // | ||
|
|
There was a problem hiding this comment.
Is this still specific to nvImageCodec v0.9.0, or does the code also work with nvImageCodec v0.6.0?
There was a problem hiding this comment.
That block was a stale v0.6.0 workaround note describing limitations that no longer apply — removed entirely in 9bd44b8. The surrounding code path is not v0.6.0-compatible anyway, since it depends on the 0.7.0+ TIFF tag metadata API.
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION. | |||
| * SPDX-FileCopyrightText: Copyright (c) 2020-2021, NVIDIA CORPORATION. | |||
There was a problem hiding this comment.
| * SPDX-FileCopyrightText: Copyright (c) 2020-2021, NVIDIA CORPORATION. | |
| * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION. |
There was a problem hiding this comment.
Fixed. This one had actually gone backwards — the base was already 2020-2026 and the branch reverted it to 2021
| if (NOT TARGET deps::nvimgcodec) | ||
| set(NVIMGCODEC_SONAME_CANDIDATES | ||
| "libnvimgcodec.so.0.9.0" | ||
| "libnvimgcodec.so.0.8.0" |
There was a problem hiding this comment.
In the conda yaml file you set requirements >= 0.9 for nvimagecodec. Why do we need 0.8 here then?
There was a problem hiding this comment.
You're right, that 0.8.0 line shouldn't be there — it's left over from when this branch still supported 0.8. Everything else already asks for 0.9: dependencies.yaml, the conda environment files, conda_build_config.yaml and pyproject.toml are all >=0.9.0,<0.10.0, and the vendored headers say NVIMGCODEC_VER_MINOR 9. Dropped it from this file and the cuslide2 copy.
Digging into it though, removing that line isn't quite enough. The list still ends with libnvimgcodec.so.0 and libnvimgcodec.so, and those symlinks point at whatever major-0 build happens to be installed — so on a machine with only 0.8 we'd still pick it up without noticing. Combined with the two things you spotted elsewhere (limit_images gone in 0.9, no compile-time version checks), that would blow up at runtime instead of failing the build, which is a much worse way to find out.
So I added a check at configure time: it reads NVIMGCODEC_VER_MAJOR/MINOR out of the nvimgcodec_version.h we discover, errors below 0.9, warns if it's outside the pinned range, and skips quietly when the header isn't there so wheel and system installs keep working. The find_package branch now looks at nvimgcodec_VERSION too — it wasn't checking anything at all before.
On consolidating the two nvimgcodec.cmake files: fair point, and the check I just added is now duplicated as well. I'd rather do that separately than grow this PR — the two files get picked up through different superbuild_depend() search paths (cuslide2 via SUPERBUILD_ADDITIONAL_DEPS_DIRS, the other via CMAKE_SUPERBUILD_DEPS_ROOT_DIR, which points at the cuslide plugin's cmake dir and has no nvimgcodec entry), so merging them takes a little care not to bbreak standalone plugin builds.
|
|
||
| set(NVIMGCODEC_SONAME_CANDIDATES | ||
| "libnvimgcodec.so.0.9.0" | ||
| "libnvimgcodec.so.0.8.0" |
There was a problem hiding this comment.
Similar question as above. Also maybe the cmake structure can be simplified so nvimagecodec search is done in a single place? But this is a question for a future pull request
There was a problem hiding this comment.
Same as on the other cmake file — 0.8.0 is gone in 0912636, with a configure-time >=0.9.0 check added.
On consolidating the two nvimgcodec.cmake files: agreed it's worth doing, but I'd rather keep that as a follow-up. They're reached through different superbuild_depend() search paths, so merging them cleanly without breaking the standalone plugin build is a bit more than this PR should take on.
| nvimgcodecCodeStreamView_t view{}; | ||
| view.struct_type = NVIMGCODEC_STRUCTURE_TYPE_CODE_STREAM_VIEW; | ||
| view.struct_size = sizeof(nvimgcodecCodeStreamView_t); | ||
| view.struct_size = offsetof(nvimgcodecCodeStreamView_t, limit_images); |
There was a problem hiding this comment.
This doesn't look right. Why would we ask for offset of struct member instead of the whole struct size? And also limit_images was removed in 0.9 so I have no idea how is this working right now
There was a problem hiding this comment.
You're right on both counts. struct_size should be sizeof(nvimgcodecCodeStreamView_t) — the offsetof(..., limit_images) was an old ABI shrink so pre-limit_images libraries wouldn't read past their known layout. That doesn't make sense in a 0.9-only PR, and limit_images is gone in the real 0.9 headers anyway.
Switched all four sites (three in nvimgcodec_decoder.cpp, one in nvimgcodec_tiff_parser.cpp) back to sizeof, and dropped the limit_images = 0 assignment plus the ABI comment.
Why it still compiled: the vendored nvimgcodec.h in this tree still had limit_images even though it claimed 0.9. That lines up with your other comment about updating to the full new headers — done in ce00b56 (headers swapped from a real 0.9.0 install, and the plugin builds cleanly against them).
| uint32_t num_channels = ifd_info.num_channels > 0 ? ifd_info.num_channels : 3; | ||
| size_t row_stride = width * num_channels; | ||
| uint32_t num_channels = decode_spec.num_channels; | ||
| size_t row_stride = width * num_channels * decode_spec.bytes_per_sample; |
There was a problem hiding this comment.
This can overflow for large width, I would static cast widt to size_t if you store it in that type anyway
There was a problem hiding this comment.
Fixed in d7c0023 — width, num_channels, and bytes_per_sample are all uint32_t, so the product was wrapping in 32-bit before the assignment to size_t. Cast width to size_t so the multiply happens in 64-bit.
| size_t row_stride = region.width * num_channels; | ||
| const DecodePixelSpec decode_spec = resolve_decode_pixel_spec(ifd_info); | ||
| uint32_t num_channels = decode_spec.num_channels; | ||
| size_t row_stride = region.width * num_channels * decode_spec.bytes_per_sample; |
There was a problem hiding this comment.
Same fix applied here in d7c0023, plus the matching ssite in schedule_batch_decode.
| ifd->height_ = height_l0 / downsample; | ||
| } | ||
| // Fix width and height of IFD | ||
| ifd->width_ = width_l0 / downsample; |
There was a problem hiding this comment.
Fixed in 1328a76 — that downsample > 0 guard was already present and got dropped when this block moved during the OME refactor. Restored it
| // | ||
|
|
||
| // 34 is from `Attribute Name="PIM_DP_IMAGE_DATA"` | ||
| char* data_ptr = const_cast<char*>(image_desc_cstr) + node_offset + 34; |
There was a problem hiding this comment.
What if this is outside of the image_desc_cstr?
There was a problem hiding this comment.
Fixed in bafbdb5. You're right that it could be — node_offset was only checked for >= 0, never against the length, so the + 34 could put us past the terminator, and both loop stop conditions (> and \0) are byte values inside the buffer, so the scan would just walk off the end of the string allocation.
This turned out to be a revert rather than a new bug: the bounds checks existed before this branch and got lost in a rebase. Restored the length check
| auto add_owned_channel_name = [&resource, &channel_names](const std::string& name) { | ||
| void* raw = resource.allocate(name.size() + 1, alignof(char)); | ||
| auto* buf = static_cast<char*>(raw); | ||
| memcpy(buf, name.c_str(), name.size() + 1); |
There was a problem hiding this comment.
I would just make the channel_names to store string instead of the string_view. That vector is the owner of the names, so this even sounds like a more correct approach.
There was a problem hiding this comment.
Addressed in eac5aee — channel names are now built in an owning std::vector<std::string>.
We still convert to std::pmr::vector<std::string_view> at the end because ImageMetadata::channel_names takes that type and may retain pointers into the metadata buffer. With _GLIBCXX_USE_CXX11_ABI=0 we also can't use std::pmr::string, so the one-shot copy into resource is what keeps the views alive for the metada
| gpu_time = time.time() - start | ||
| gpu_np = _to_numpy(gpu_region) | ||
| print(f" GPU level {level}: {gpu_np.shape}, {gpu_np.dtype}, {gpu_time:.4f}s") | ||
| if gpu_np.shape != cpu_np.shape: |
There was a problem hiding this comment.
shape check is not enough. I would check the pixels too. And maybe you should check that each level have a correct dimensions (that they decrease with correct ratio for each level done) - otherwise you will not be sure that parsing is working as expected.
There was a problem hiding this comment.
Addressed in 84fe4ed. Multi-level GPU/CPU reads now also do np.array_equal (with a max-diff error on mismatch), and _validate_pyramid_geometry asserts that each level shrinks, downsamples increase, and dims match base / ds within ±1 for rounding.let me know if further testign is needed
| print(f" ⚠️ GPU plane validation skipped/failed: {e}") | ||
|
|
||
|
|
||
| def _validate_batch_decode(img, level_dims): |
There was a problem hiding this comment.
You added a bunch of code for higher resolution decode (with dtype uint 16) - I would add test for such case as well
There was a problem hiding this comment.
Addressed in 15752d3. Added a synthetic uint16 pyramidal OME-TIFF decode check that always runs: it asserts metadata/region dtype stays uint16, preserves values >255 (vso we didn't silently narrow to uint8), and matches source pixels on level 0 and level 1. When the caller-provided file is already uint16, the multi-level CPU/GPU checks now enforce that dtype too.
max_num_cpu_threads = 0 requests one CPU thread per core from nvImageCodec's default executor. That was harmless when this decoder was CPU-only and used solely for metadata extraction, but it now backs the primary decode path (NVIMGCODEC_DEVICE_CURRENT, all backends enabled), so every worker process allocated a full-core threadpool and multi-worker runs regressed into the thread over-contention this heuristic was originally added to prevent. Restore compute_max_decoder_threads(), which bounds the pool to min(max(1, hardware_concurrency / 4), 8) and stays overridable via CUCIM_MAX_DECODER_THREADS. Also drop the stale nvImageCodec v0.6.0 limitation comments: individual TIFF tags are queried unconditionally through NVIMGCODEC_METADATA_KIND_TIFF_TAG by extract_tiff_tags(), so the claim that they are unavailable contradicted the adjacent code.
Replace the checked-in nvimgcodec.h / nvimgcodec_version.h copies with the real 0.9.0 package headers. The previous vendored files claimed 0.9 but still described the pre-0.9 CodeStreamView layout that included limit_images, so call sites could compile against an API the linked library no longer provides. With the real headers in place, restore struct_size to sizeof(nvimgcodecCodeStreamView_t) at all four CodeStreamView sites and drop the leftover limit_images assignment and offsetof ABI shrink. Also bump SPDX copyright years on files touched by this PR.
|
/ok to test ce00b56 |
width, num_channels and bytes_per_sample are all uint32_t, so the row stride product was evaluated in 32-bit and could wrap before being widened on assignment to size_t. Cast the width operand to size_t so the multiply happens in 64-bit, at all three decode sites.
The unconditional `+ 34` skip past the PIM_DP_IMAGE_DATA node could land beyond the terminator, leaving the following scan loops unanchored since both stop conditions are byte values inside the buffer. Restore the length check on node_offset and the explicit desc_end sentinel so every loop is bounded by the string, and drop the skip and the const_cast.
Replace the allocate-and-view lambda with an owning vector of strings. ImageMetadata::channel_names still takes string_views and may retain pointers into the metadata buffer, so copy into the resource once at the end when constructing the view vector the API requires.
|
/ok to test eac5aee |
Add pixel-level CPU/GPU equality checks on multi-level reads, and assert pyramid dimensions shrink and match the reported downsample factors so parsing mistakes cannot pass on shape alone.
Bump SPDX copyright years, normalize trailing newlines, convert the vendored nvImageCodec headers to the SPDX-only form the copyright hook expects, and ignore upstream typos (minize/shrinked) in those headers.
|
/ok to test aef0ca1 |
Add a synthetic uint16 OME-TIFF decode path that asserts metadata and region dtypes stay 16-bit, preserves values above 255, and matches source pixels across pyramid levels. Also enforce uint16 on multi-level CPU/GPU checks when the input file reports that dtype.
|
/ok to test 15752d3 |
The geometry check assumed a single-plane WSI pyramid: strictly increasing downsample per level, and level dims recoverable as base / downsample. Both assumptions are false in practice. cuslide2 reports downsample as the mean of the per-axis ratios ((w0/wi) + (h0/hi)) / 2, and levels may use different X and Y ratios, so dims cannot be derived from the scalar. Multi-plane OME files also expose one IFD per C/Z/T plane, so many levels legitimately share a resolution. Validate what cuslide2 actually guarantees instead: dims are non-increasing, downsample strictly increases when the resolution changes and is identical when a resolution repeats, and the reported scalar matches the mean axis ratio. Repeated resolutions now warn that the file looks multi-plane rather than failing the run.
|
/ok to test aeca33b |
Resolution was hardcoded to 1.0 with no unit, so every file reported meaningless pixel spacing. Query tags 282/283/296, decode RATIONAL values, and populate the IFD fields from them. OME TiffData entries only reference full-resolution planes, so mapping planes alone collapsed the level list to level 0 and made pyramid sub-resolutions unreachable through read_region(). Backfill the levels the plane index does not cover. Also surface two previously silent failures: tag retrieval returning nothing (which disables OME/vendor detection) and unusable OME XML. Fix the test helper so device-resident results route through CuPy instead of degrading every GPU comparison to a skipped check.
Reading a multi-plane OME-TIFF aborted with "free(): invalid pointer" after the run completed, roughly one exit in five. The parser manager destroyed its nvImageCodec decoder at process exit. That cannot be ordered against the CUDA driver's own teardown, so nvimgcodecDecoderDestroy intermittently freed pointers the driver had already reclaimed. Neither available ordering was safe: the singleton's static destructor runs before the atexit handler registered in the constructor, because that handler is registered first and exit handlers run in reverse order, and moving teardown to the handler still aborted. Leak the singleton and skip exit-time teardown. These are process- lifetime handles, so the driver and OS reclaim them. shutdown() stays available for deterministic release while CUDA is up.
read_region() threw "Failed to decode IFD[0] with nvImageCodec" for a region with no overlap with the image, where the documented behavior and the cuslide behavior is an all-zero raster of the requested size. This failed test_tiff_stripe_outside for all three compression variants. nvImageCodec fills out-of-bounds pixels when a region partially overlaps, which is why boundary reads worked, but it rejects a region that does not intersect the image at all. Detect that case and return a zeroed raster without consulting the decoder, for host and device outputs alike.
|
/ok to test 729d27a |
|
|
||
| // Fallback: file extension heuristics when COMPRESSION tag is not present in file | ||
| // Fallback: file extension heuristics when COMPRESSION tag is not available | ||
| // (either nvImageCodec < 0.7.0 or tag not present in file) |
There was a problem hiding this comment.
Is comment still relevant? As far as I understand it is not possible to have nvImageCodec < 0.7.0 now
|
/ok to test 729d27a |
Packaging now pins nvImageCodec to >=0.9.0,<0.10.0, so comments that gate behavior on 0.6.0, 0.7.0, or 0.8.0 no longer describe anything reachable. Reword them to say what the code does rather than which release introduced it, keeping the vendor-detection fallbacks: those still matter for files that carry no SOFTWARE tag or ImageDescription, independently of the codec version. Also remove CUSLIDE2_NVIMGCODEC_HAS_TIFF_TAG_METADATA. It was defined unconditionally to 1 with no alternate branch, so the surrounding preprocessor conditionals were always taken.
|
/ok to test e011f44 |
What this PR changes
ome_xml.h/.cpp)TiffDatacompanion-file referencesTiffDataonly references full-resolution planescuslidefree()Scope notes
Validation
cucim.kit.cuslide2successfully in the prepared build environmentcuslide2_testspre-commit run --hook-stage manual --all-filespassesRunning the pyramidal OME-TIFF script
The script needs no GPU-specific flags: it decodes each region on both CPU and
GPU and compares the results, so a GPU is required for the comparison checks to
run rather than be skipped. Pass
--no-cacheto exercise the direct decode pathinstead of the tile cache.
If you rebuild the C++ but see unchanged behavior under
pytest, note that thePython package loads the plugin from
python/cucim/src/cucim/clara/, not fromcpp/plugins/cucim.kit.cuslide2/build-release/lib/. Re-running./run build_localrefreshes that copy.Expected output, multi-plane OME-TIFF
retina_large.ome.tiff, 2 resolution levels,SizeC=2,SizeZ=64, uint16:Two details worth checking in that output.
Levels: 2confirms the pyramidsub-resolution survives: OME
TiffDataentries reference only thefull-resolution planes, so mapping planes alone collapses the list to level 0 and
makes level 1 unreachable through
read_region(). And the run must end at✅ Pyramidal OME-TIFF test completedwith exit status 0; a trailingfree(): invalid pointer/Abortedwas the exit-time teardown crash fixed here,which reproduced on roughly one run in five.
Expected output, non-OME pyramidal TIFF
The same script accepts ordinary WSIs. Against
CMU-1.tifthe OME and planechecks report as skipped, which is correct for a file with no OME-XML:
Note that per-axis downsample ratios differ on real WSIs, so the geometry check
compares against the mean of the axis ratios rather than assuming a single factor.
Unit tests
cd python/cucim ENABLE_CUSLIDE2=1 python -m pytest tests/unit/clara/test_tiff_read_region.py -qAll 26 pass with
cuslide2enabled and with the default plugin. The threetest_tiff_stripe_outsidecases previously failed undercuslide2withFailed to decode IFD[0] with nvImageCodec. ROI: (-48,-40) 16x16: nvImageCodecfills out-of-bounds pixels for a partially overlapping region, which is why
boundary reads passed, but rejects a region that does not intersect the image at
all.